Skip to content

Add named params - first iteration #1019

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 26 commits into from

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Jan 12, 2016

This PR adds named class parameters [type A] and named arguments to
applied types T[A = U].

This is not quite complete yet. We are still missing named arguments in method applications and patterns. Also, we need more tests in particular in what concerns type inference.

@DarkDimius
Copy link
Contributor

[info] !! 222 - pos/printing_v1 [compilation failed]

That's strange, as the very same file succeeded being compiled by unit-tests.

@odersky
Copy link
Contributor Author

odersky commented Jan 12, 2016

/rebuild

@odersky
Copy link
Contributor Author

odersky commented Jan 12, 2016

We get OutOfMemoryErrors (GC overhead limit exceeded). Is there something one can do to fix this?

@odersky
Copy link
Contributor Author

odersky commented Jan 12, 2016

/rebuild

@odersky
Copy link
Contributor Author

odersky commented Jan 12, 2016

/rebuild -- trying to find out whether this is random or not.

@odersky
Copy link
Contributor Author

odersky commented Jan 13, 2016

/rebuild

@odersky
Copy link
Contributor Author

odersky commented Jan 17, 2016

rebased to master

@odersky
Copy link
Contributor Author

odersky commented Jan 17, 2016

We see:

Native memory allocation (mmap) failed to map 2463629312 bytes for committing reserved memory.

@odersky
Copy link
Contributor Author

odersky commented Jan 17, 2016

Tests pass locally.

@odersky
Copy link
Contributor Author

odersky commented Jan 17, 2016

/rebuid

@odersky
Copy link
Contributor Author

odersky commented Jan 17, 2016

/rebuild

@smarter
Copy link
Member

smarter commented Jan 17, 2016

Try rebasing on top of master to get rid of the errors related to t7880.scala

@smarter
Copy link
Member

smarter commented Jan 18, 2016

Note that no junit build will succeed until scala/scala-jenkins-infra#158 is merged and deployed, sorry about that.

@odersky odersky force-pushed the add-named-params branch 2 times, most recently from 4ad5a69 to e48ecb0 Compare January 20, 2016 18:09
@odersky
Copy link
Contributor Author

odersky commented Feb 5, 2016

Get this in now? It's been lingering for a long time.

@odersky
Copy link
Contributor Author

odersky commented Feb 8, 2016

Rebased to master. This is ready to go in now. I know it is a big PR but it is an important one. If there are no reviews I'll merge.

@smarter
Copy link
Member

smarter commented Feb 8, 2016

I'll do another pass on this today

in.nextToken()
otherArgs(NamedArg(name, argType()), namedTypeArg)
case firstArg =>
if (in.token == EQUALS) println(s"??? $firstArg")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use println in the compiler

Changes needed to support simple named type parameters.
Not yet implemented: named arguments.
Types#underlyingClassRef and PostTyper#normalizeTree need to be changed so they can
deal with partial named parameter lists.
After unpickling we might see an alias

    X = pre.X

where pre =:= the ThisType of the enclosing class. But it
might not be `eq` to it.
Named parameters cause some elements to be unpickled
in a different order as they are pickled. In particular
term parameter aliases and type parameter aliases seem to be swapped.
There is a before/after difference having to do with the
order in which class declarations show up.
Needs to work also if named arg refers to an abstract type,
not a parameter.
We had same fleyness in number of errors of cycle.scala
which prompted this.
Also, add an unrelated test file to pos.
Was not fixed by accident then, so we do it here now.
The current Scala spec only considers methods without default parameters
for overloading resolution (unless only a single one remains anyway after
filtering by shape). This is needlessly restrictive. But dropping this
restriction (as dotty does) can lead to ambiguity errors, which is why
run/t8197 did not compile anymore.

We fix the problem by a last try rule: If after asSpecific tests there are
still several alternatives, and only one of them is without default arguments,
pick that one.

I tried an alternative rule which would make the distinction on default params
earlier but that one fails for the overloaded tree copier functions in Trees.scala
(the method with default parameters is also the one which is more specific).
Lets one also pass named arguments to methods.
@odersky
Copy link
Contributor Author

odersky commented Feb 12, 2016

Reviewer comments so far have been addressed. Are there others coming?

@odersky
Copy link
Contributor Author

odersky commented Feb 12, 2016

Also fixes #78

@odersky
Copy link
Contributor Author

odersky commented Feb 18, 2016

Ready to merge?

@odersky
Copy link
Contributor Author

odersky commented Feb 19, 2016

Subsumed by #1072

@odersky odersky closed this Feb 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants